[C#] Remove unnecessary SemaphoreSlim locks for handler fields#625
[C#] Remove unnecessary SemaphoreSlim locks for handler fields#625stephentoub merged 2 commits intomainfrom
Conversation
…rInputHandlerLock Replace SemaphoreSlim-based locking with volatile fields for _permissionHandler and _userInputHandler. These fields are single reference types where reads and writes are atomic in .NET, so locking is unnecessary. The volatile keyword ensures proper visibility across threads. Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Cross-SDK Consistency ReviewI've reviewed this PR for consistency across all four SDK implementations (Node.js/TypeScript, Python, Go, and .NET). Summary✅ No cross-SDK consistency issues found. This is a .NET-specific optimization that correctly addresses thread-safety in a way appropriate for C#. AnalysisThis PR removes unnecessary
Comparison with Other SDKsI reviewed how the other SDKs handle these same handlers: Node.js/TypeScript:
Python:
Go:
Why This Doesn't Create InconsistencyEach language handles this pattern according to its concurrency model:
Recommendation✅ Approve - This change is correct and maintains appropriate consistency. Each SDK uses the synchronization pattern that's idiomatic and correct for its platform. Optional follow-up: The Python SDK could potentially be simplified similarly (using simple assignment instead of locks), but that would need careful analysis of Python's threading semantics and is outside the scope of this PR.
|
There was a problem hiding this comment.
Pull request overview
This PR simplifies the .NET CopilotSession handler registration/invocation logic by removing per-handler SemaphoreSlim locks and switching to volatile fields for permission and user-input handlers.
Changes:
- Removed
_permissionHandlerLock/_userInputHandlerLockand their lock-protected read/write paths. - Marked
_permissionHandlerand_userInputHandlerasvolatileand switched to direct assignment / direct read. - Simplified dispose cleanup for
_permissionHandler(now direct null assignment).
Comments suppressed due to low confidence (3)
dotnet/src/Session.cs:314
- Now that the SemaphoreSlim around _permissionHandler is removed, DisposeAsync can race with HandlePermissionRequestAsync: a request can read a non-null handler and invoke it while disposal is in progress, even though DisposeAsync sets the handler to null to disable callbacks. If the intended behavior is to stop invoking handlers once disposal begins, add an _isDisposed check (or similar) in HandlePermissionRequestAsync and/or use an atomic exchange + disposed check to prevent invocation after disposal starts.
internal async Task<PermissionRequestResult> HandlePermissionRequestAsync(JsonElement permissionRequestData)
{
var handler = _permissionHandler;
if (handler == null)
{
dotnet/src/Session.cs:353
- HandleUserInputRequestAsync reads _userInputHandler without any disposed-state check. Combined with DisposeAsync not clearing _userInputHandler, an in-flight or late userInput.request can still call back into user code after the session is disposed. Consider treating disposed sessions as unavailable here (e.g., throw ObjectDisposedException or return a deterministic failure) and ensure DisposeAsync disables this handler.
This issue also appears on line 553 of the same file.
internal async Task<UserInputResponse> HandleUserInputRequestAsync(UserInputRequest request)
{
var handler = _userInputHandler;
if (handler == null)
{
throw new InvalidOperationException("No user input handler registered");
}
dotnet/src/Session.cs:556
- DisposeAsync clears _permissionHandler but leaves _userInputHandler set. That keeps user callbacks (and any captured resources) alive after disposal and can allow user-input requests to still invoke the handler even though the session is disposed. Clear _userInputHandler (and any other handler fields) during disposal for consistent cleanup behavior.
_eventHandlers.Clear();
_toolHandlers.Clear();
_permissionHandler = null;
_permissionHandlerLockand_userInputHandlerLockusage indotnet/src/Session.cs_permissionHandlerLockSemaphoreSlim field declaration_userInputHandlerLockSemaphoreSlim field declaration_permissionHandlerasvolatile_userInputHandlerasvolatileRegisterPermissionHandlerto direct assignmentHandlePermissionRequestAsyncto direct readRegisterUserInputHandlerto direct assignmentHandleUserInputRequestAsyncto direct read_permissionHandlerto direct assignment✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.